Fix tokio bash timeout buffered output capture#42
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #42 +/- ##
==========================================
+ Coverage 74.84% 75.13% +0.28%
==========================================
Files 67 67
Lines 1980 2019 +39
==========================================
+ Hits 1482 1517 +35
- Misses 498 502 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Sewer56
force-pushed
the
fix/tokio-timeout-buffered-output
branch
2 times, most recently
from
dd56e3b to
d685824
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds asynchronous pipe-draining infrastructure (PipeDrainTask, SharedPipeBuffer, spawn/take/await helpers), constants for drain behavior, and a helper to build timeout errors that include buffered stdout/stderr. Refactors execute_command to spawn concurrent drains for stdout/stderr, race process completion against a timeout, kill the process tree on timeout, and continue draining with a grace period before returning either aggregated BashOutput or a timeout error containing buffered output. Adds tests covering timeout buffered output, grace abort behavior, large-output handling, exit codes, and invalid working directories. Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs`:
- Around line 35-44: The await_pipe_drain_task_with_grace function currently
aborts the JoinHandle<Vec<u8>> on timeout which drops the task-local Vec<u8>
buffer; remove the task.abort() call in the tokio::select! branch that handles
tokio::time::sleep(grace) so the code instead simply awaits the task (i.e.,
return task.await.unwrap_or_default()) to preserve any buffered output; if you
still need strict timeout semantics, refactor the drain task to write into a
shared/external buffer (rather than task-local state) so it can be read after
aborting.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin, false)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc, false)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu, false)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by calling.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Usememchrcrate for fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Use [TypeName] rustdoc links instead of backticks in documentation
Files:
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs
🧬 Code graph analysis (1)
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs (1)
src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs (1)
execute_command(17-125)
🔇 Additional comments (3)
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs (3)
17-27: Good drain-task setup with preallocation.This is a solid change: drains are decoupled and the buffer is preallocated for the expected pipe size.
As per coding guidelines,
src/**/*.rs: “Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)”.
48-75: Timeout error construction is clear and useful.Including buffered stdout/stderr in the timeout message is a practical UX improvement for debugging failed commands.
231-247: Nice regression test for buffered timeout output.This test directly validates the intended behavior change and protects against regressions in timeout-output preservation.
Sewer56
force-pushed
the
fix/tokio-timeout-buffered-output
branch
3 times, most recently
from
9e4bc83 to
c770087
Compare
Avoid canceling stdout/stderr drains by spawning independent pipe-read tasks and racing only child completion against timeout. On timeout, kill the process tree, await drain tasks briefly to capture buffered output, and include captured stdout/stderr in timeout errors; add a regression test for pre-timeout output.
Sewer56
force-pushed
the
fix/tokio-timeout-buffered-output
branch
from
c770087 to
f5ea4b8
Compare
Fix tokio bash timeout buffered output capture
1 participant
Summary
llm-coding-tools-core/src/tools/bash/tokio_impl.rsto use a sharedArc<Mutex<Vec<u8>>>buffer so aborting the drain task cannot drop already-read bytes.grace_abort_retains_shared_pipe_bufferto verify buffered data survives the abort path.Testing
.cargo/verify.sh